-
-
Notifications
You must be signed in to change notification settings - Fork 35.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Geometry: Move computeLineDistance() to Line/LineSegments #13147
Conversation
examples/canvas_lines_dashed.html
Outdated
|
||
var geometryCube = cube( 50 ); | ||
|
||
geometryCube.computeLineDistances(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW: With CanvasRenderer
it is not necessary to compute the line distances for dashed lines...
|
I wonder if we could "hide" the call of So if the renderer detects an instance of |
|
||
if ( geometry.isBufferGeometry ) { | ||
|
||
// we assume non-indexed geometry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you think you should handle indexed geometry gracefully?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. It should be better with 0286198
src/objects/Line.js
Outdated
|
||
} | ||
|
||
lineDistances.push( distance ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish we did not always create these JS arrays unnecessarily. It is a pattern we use over-and-over...
To do so, the distances need to be calculated differently for line segments. |
What would be different? I thought we calculate for each point the distance to the beginning of the line... |
We should be calculating the cumulative length. |
Ah, now i understand. When using |
src/objects/Line.js
Outdated
|
||
geometry.lineDistances[ i ] = distance; | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
geometry.lineDistances[ 0 ] = 0;
for ( var i = 1, l = vertices.length; i < l; i ++ ) {
geometry.lineDistances[ i ] += vertices[ i - 1 ].distanceTo( vertices[ i ] );
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That did not work for me since geometry.lineDistances[ i ]
needs to be initialized with the previous value. Otherwise the summation does not work.
geometry.lineDistances[ 0 ] = 0;
for ( var i = 1, l = vertices.length; i < l; i ++ ) {
geometry.lineDistances[ i ] = geometry.lineDistances[ i - 1 ];
geometry.lineDistances[ i ] += vertices[ i - 1 ].distanceTo( vertices[ i ] );
}
Related: #8494 When the PR is merged, there is one less reason for users to convert their lines based on |
Thanks! |
N.B. |
Even if we compute the distance from the last vertex to the first one, how would we put this value in the lineDistances array/attribute? Normally we have for each vertex a respective distance value. This seems not to be true for |
Right. That's the problem. |
This PR moves
.computeLineDistance()
toLine
and adds support for (non-indexed)BufferGeometry
. The combination ofLineDashedMaterial
andBufferGeometry
does work now. This feature was frequently asked by users, e.g.https://discourse.threejs.org/t/computelinedistances-with-buffergeometry/644 or
#7013
The implementation is based on @WestLangley's suggestion, see #7013 (comment)